Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix incorrect ternary state computation for bank toggles #28746

Merged
merged 2 commits into from
Jul 5, 2024

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Jul 5, 2024

Closes #28741.

Regressed in a7b066f.

The intent of the original change there was to ensure that addition banks being set will put the ternary state toggles in indeterminate state (to at least provide a visual indication that the selection does not use a single bank). This would previously not be the case due to the use of .All() in the original condition (a single object/node was considered to have a bank enabled if and only if all samples within it used it). However the attempt to fix that via switching to Any() was not correct.

The logic used in the offending commit operates on extracted Samples and NodeSamples from the selection, and would consider the ternary toggle:

  • fully off if none of the samples/node samples contained a sample with the given bank,
  • indeterminate if the some of the samples/node samples contained a sample with the given bank,
  • fully on if at least one sample from every samples/node samples contained a sample with the given bank.

This is a two-tiered process, as in first a binary on/off state is extracted from each object's samples/node samples, and then a ternary state is extracted from all objects/nodes. This is insufficient to express the desired behaviour, which is that the toggle should be:

  • fully off if none of the individual samples in the selection use the given bank,
  • indeterminate if at least one individual sample in the selection uses the given bank,
  • fully on if all individual samples in the selection use the given bank.

The second wording is flattened, and no longer tries to consider "nodes" or "objects", it just looks at all of the samples in the selection without concern as to whether they're from separate objects/nodes or not.

To explain why this discrepancy caused the bug, consider a single object with a soft normal bank and drum addition bank. Selecting the object would cause a ternary button state update; as per the incorrect logic, there were two samples on the object and each had its own separate banks, so two ternary toggles would have their state set to True (rather than the correct Indeterminate), thus triggering a bindable feedback loop that would cause one of these banks to win and actually overwrite the other.

Note that the addition indeterminate state computation still needs to do the two-tiered process, because there it actually makes sense (for a selection to have an addition fully on rather than indeterminate, every object/node must contain that addition).

bdach added 2 commits July 5, 2024 08:54
Closes ppy#28741.

Regressed in a7b066f.

The intent of the original change there was to ensure that addition
banks being set will put the ternary state toggles in indeterminate
state (to at least provide a visual indication that the selection does
not use a single bank). This would previously not be the case due to
the use of `.All()` in the original condition (a single object/node
was considered to have a bank enabled if and only if *all* samples
within it used it). However the attempt to fix that via switching
to `Any()` was not correct.

The logic used in the offending commit operates on extracted `Samples`
and `NodeSamples` from the selection, and would consider the ternary
toggle:

- fully off if none of the samples/node samples contained a sample with
  the given bank,
- indeterminate if the some of the samples/node samples contained a
  sample with the given bank,
- fully on if at least one sample from every samples/node samples
  contained a sample with the given bank.

This is a *two-tiered* process, as in first a *binary* on/off state is
extracted from each object's samples/node samples, and *then* a ternary
state is extracted from all objects/nodes. This is insufficient to
express the *desired* behaviour, which is that the toggle should be:

- fully off if *none of the individual samples in the selection* use
  the given bank,
- indeterminate if *at least one individual sample in the selection*
  uses the given bank,
- fully on if *all individual samples in the selection* use the given
  bank.

The second wording is flattened, and no longer tries to consider "nodes"
or "objects", it just looks at all of the samples in the selection
without concern as to whether they're from separate objects/nodes
or not.

To explain why this discrepancy caused the bug, consider a single object
with a `soft` normal bank and `drum` addition bank. Selecting the object
would cause a ternary button state update; as per the incorrect logic,
there were two samples on the object and each had its own separate
banks, so two ternary toggles would have their state set to `True`
(rather than the correct `Indeterminate`), thus triggering a bindable
feedback loop that would cause one of these banks to win and actually
overwrite the other.

Note that the addition indeterminate state computation *still* needs
to do the two-tiered process, because there it actually makes sense (for
a selection to have an addition fully on rather than indeterminate,
*every* object/node *must* contain that addition).
@bdach bdach added area:editor next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! labels Jul 5, 2024
@bdach bdach self-assigned this Jul 5, 2024
@smoogipoo smoogipoo merged commit 0bb6c25 into ppy:master Jul 5, 2024
15 of 17 checks passed
@bdach bdach deleted the fix-ternary-state-breakage branch July 8, 2024 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:editor next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! size/S
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Selecting object with different normal and addition banks arbitrarily unifies them
2 participants